Fix setting ServerAddress property in NativeStore #4655
Conversation
thaJeztah
added
status/2-code-review
kind/bugfix
|
Thanks! We also merged a fix on the daemon-side in moby/moby#46779 Perhaps both won't do harm though /cc @dmcgowan |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4655 +/- ##
=======================================
Coverage 59.74% 59.75%
=======================================
Files 288 288
Lines 24849 24853 +4
=======================================
+ Hits 14846 14850 +4
Misses 9117 9117
Partials 886 886 |
| @@ -76,6 +77,7 @@ func (c *nativeStore) GetAll() (map[string]types.AuthConfig, error) { | |||
| ac.Username = creds.Username | |||
| ac.Password = creds.Password | |||
| ac.IdentityToken = creds.IdentityToken | |||
| ac.ServerAddress = creds.ServerAddress | |||
There was a problem hiding this comment.
is ac.ServerAddress ever a non-empty value?
There was a problem hiding this comment.
I do not believe so no. Since in both stores the code always sets it to what the Registry Hostname is. Is your idea to just have creds be returned there directly? 🙂
There was a problem hiding this comment.
@dmcgowan do you mean you prefer to make this conditional to keep that option open?
Something like this?;
if ac.ServerAddress == "" {
ac.ServerAddress = creds.ServerAddress
}There was a problem hiding this comment.
I had not read it that way yet. That makes sense. So we only set it when it is empty and not accidentally overwrite a potential value. I can add that real quick.
|
@thaJeztah agreed, both changes make sense |
This will return the ServerAddress property when using the NativeStore.
This happens when you use docker credential helpers, not the credential
store.
The reason this fix is needed is because it needs to be propagated
properly down towards `moby/moby` project in the following logic:
```golang
func authorizationCredsFromAuthConfig(authConfig registrytypes.AuthConfig) docker.AuthorizerOpt {
cfgHost := registry.ConvertToHostname(authConfig.ServerAddress)
if cfgHost == "" || cfgHost == registry.IndexHostname {
cfgHost = registry.DefaultRegistryHost
}
return docker.WithAuthCreds(func(host string) (string, string, error) {
if cfgHost != host {
logrus.WithFields(logrus.Fields{
"host": host,
"cfgHost": cfgHost,
}).Warn("Host doesn't match")
return "", "", nil
}
if authConfig.IdentityToken != "" {
return "", authConfig.IdentityToken, nil
}
return authConfig.Username, authConfig.Password, nil
})
}
```
This logic resides in the following file :
`daemon/containerd/resolver.go` .
In the case when using the containerd storage feature when setting the
`cfgHost` variable from the `authConfig.ServerAddress` it will always be
empty. Since it will never be returned from the NativeStore currently.
Therefore Docker Hub images will work fine, but anything else will fail
since the `cfgHost` will always be the `registry.DefaultRegistryHost`.
Signed-off-by: Eric Bode <[email protected]>
StealthyCoder
force-pushed
the
4653-fix-credential-helper
branch
from
652cd45 to
b24e7f8
Compare
This will return the ServerAddress property when using the NativeStore. This happens when you use docker credential helpers, not the credential store.
The reason this fix is needed is because it needs to be propagated properly down towards
moby/mobyproject in the following logic:This logic resides in the following file :
daemon/containerd/resolver.go.In the case when using the containerd storage feature when setting the
cfgHostvariable from theauthConfig.ServerAddressit will always be empty. Since it will never be returned from the NativeStore currently. Therefore Docker Hub images will work fine, but anything else will fail since thecfgHostwill always be theregistry.DefaultRegistryHost.- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)